Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ Issue 548 ] Production Cert for Simpler Domain #557

Merged
merged 8 commits into from
Sep 29, 2023
Merged

[ Issue 548 ] Production Cert for Simpler Domain #557

merged 8 commits into from
Sep 29, 2023

Conversation

SammySteiner
Copy link
Contributor

Summary

Fixes #548

Time to review: 5 mins

Changes proposed

  • Add frontend prod certificate
  • Update ternary to send cert to service module

@SammySteiner
Copy link
Contributor Author

Here's the results from running terraform plan
In Dev there were 0 changes.

This was the prod output:

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # module.service.aws_lb_listener.alb_listener_https[0] will be created
  + resource "aws_lb_listener" "alb_listener_https" {
      + arn               = (known after apply)
      + certificate_arn   = "arn:aws:acm:us-east-1:315341936575:certificate/474db8f9-913b-4002-b384-2fccdba29e97"
      + id                = (known after apply)
      + load_balancer_arn = "arn:aws:elasticloadbalancing:us-east-1:315341936575:loadbalancer/app/frontend-prod/6c8eba5daa95bfe2"
      + port              = 443
      + protocol          = "HTTPS"
      + ssl_policy        = "ELBSecurityPolicy-TLS13-1-2-2021-06"
      + tags_all          = {
          + "description"         = "Application resources created in prod environment"
          + "environment"         = "prod"
          + "owner"               = "navapbc"
          + "project"             = "grants-equity"
          + "repository"          = "https://github.com/HHS/grants-equity"
          + "terraform"           = "true"
          + "terraform_workspace" = "default"
        }

      + default_action {
          + order = (known after apply)
          + type  = "fixed-response"

          + fixed_response {
              + content_type = "text/plain"
              + message_body = "Not Found"
              + status_code  = "404"
            }
        }
    }

  # module.service.aws_lb_listener_rule.app_https_forward[0] will be created
  + resource "aws_lb_listener_rule" "app_https_forward" {
      + arn          = (known after apply)
      + id           = (known after apply)
      + listener_arn = (known after apply)
      + priority     = 100
      + tags_all     = {
          + "description"         = "Application resources created in prod environment"
          + "environment"         = "prod"
          + "owner"               = "navapbc"
          + "project"             = "grants-equity"
          + "repository"          = "https://github.com/HHS/grants-equity"
          + "terraform"           = "true"
          + "terraform_workspace" = "default"
        }

      + action {
          + order            = (known after apply)
          + target_group_arn = "arn:aws:elasticloadbalancing:us-east-1:315341936575:targetgroup/app-20230818191911008700000001/7b0dd95f0b5fdb3c"
          + type             = "forward"
        }

      + condition {
          + path_pattern {
              + values = [
                  + "/*",
                ]
            }
        }
    }

  # module.service.aws_lb_listener_rule.redirect_http_to_https[0] will be created
  + resource "aws_lb_listener_rule" "redirect_http_to_https" {
      + arn          = (known after apply)
      + id           = (known after apply)
      + listener_arn = "arn:aws:elasticloadbalancing:us-east-1:315341936575:listener/app/frontend-prod/6c8eba5daa95bfe2/c6867f9523cac9f6"
      + priority     = 100
      + tags_all     = {
          + "description"         = "Application resources created in prod environment"
          + "environment"         = "prod"
          + "owner"               = "navapbc"
          + "project"             = "grants-equity"
          + "repository"          = "https://github.com/HHS/grants-equity"
          + "terraform"           = "true"
          + "terraform_workspace" = "default"
        }

      + action {
          + order = (known after apply)
          + type  = "redirect"

          + redirect {
              + host        = "#{host}"
              + path        = "/#{path}"
              + port        = "443"
              + protocol    = "HTTPS"
              + query       = "#{query}"
              + status_code = "HTTP_301"
            }
        }

      + condition {
          + path_pattern {
              + values = [
                  + "/*",
                ]
            }
        }
    }

  # module.service.aws_security_group_rule.https_ingress[0] will be created
  + resource "aws_security_group_rule" "https_ingress" {
      + cidr_blocks              = [
          + "0.0.0.0/0",
        ]
      + description              = "Allow HTTPS traffic from public internet"
      + from_port                = 443
      + id                       = (known after apply)
      + protocol                 = "tcp"
      + security_group_id        = "sg-086c755626f036ba0"
      + security_group_rule_id   = (known after apply)
      + self                     = false
      + source_security_group_id = (known after apply)
      + to_port                  = 443
      + type                     = "ingress"
    }

Plan: 4 to add, 0 to change, 0 to destroy.

@SammySteiner SammySteiner marked this pull request as ready for review September 28, 2023 16:13
@SammySteiner
Copy link
Contributor Author

I ran the apply and it seems to have successfully attached the cert to the prod load balancer:
image

daphnegold
daphnegold reviewed Sep 28, 2023
View reviewed changes
infra/frontend/service/main.tf Outdated
Comment on lines 111 to 115
cert_arn = (
terraform.workspace != "default" ? null :
var.environment_name == "prod" ? data.aws_acm_certificate.frontend_prod_cert.arn :
data.aws_acm_certificate.frontend_dev_cert.arn
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cert_arn = (
terraform.workspace != "default" ? null :
var.environment_name == "prod" ? data.aws_acm_certificate.frontend_prod_cert.arn :
data.aws_acm_certificate.frontend_dev_cert.arn
)
# Temporary to avoid issues if Terratest if workspace is default and pass correct environment cert
cert_arn = (
terraform.workspace != "default" ? null :
(var.environment_name == "prod" ? data.aws_acm_certificate.frontend_prod_cert.arn :
data.aws_acm_certificate.frontend_dev_cert.arn)
)

Copy link
Contributor

@daphnegold daphnegold Sep 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listing some ideas to simplify this ternary if you want, encapsulating all the thoughts of the day:

Option 1: Put the domains into a map in /app-config/main.ts locals and use it in env_config module to pass the correct environment's domain name to service.

Option 2: Put domain name in dev.tfvars and prod.tfvars 😆 I totally forgot these existed.

For example, in dev.tfvars
domain_name = "beta.grants.gov"

Then in service use it as var.domain_name.

daphnegold
daphnegold approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@daphnegold daphnegold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of the ternary, and it'll get uglier when/if we add staging, but 🚢.

Thanks for your patience as I think out possible solutions to simplifying the ternary. I distilled all the thoughts down into one comment and happy to continue to work on implementing that with you if you'd like.

@SammySteiner
Copy link
Contributor Author

Thanks for the pairing and discussion both on and off github @daphnegold! I updated it to use the tfvars file so it's much cleaner now and we can set the domains in a much more reasonable place.

daphnegold
daphnegold approved these changes Sep 29, 2023
View reviewed changes
Copy link
Contributor

@daphnegold daphnegold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love that there's no need to check for default workspace to avoid Terratest issues now. You got rid of all the yuck, nice going.

@SammySteiner SammySteiner merged commit ebb3228 into main Sep 29, 2023
@SammySteiner SammySteiner deleted the sammysteiner/issue-548-simpler-cert branch September 29, 2023 17:54
@coilysiren coilysiren mentioned this pull request Oct 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daphnegold daphnegold daphnegold approved these changes

Assignees

@acouch acouch

@daphnegold daphnegold

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Task]: Update SSL certificate for move to "simpler.grants.gov"
3 participants
@SammySteiner @daphnegold @acouch